✨ Add the support for capacity blocks #5211

athiruma · 2024-11-08T07:41:22Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR added the support for CapacityBlocks for ML, by adding this we can get the support for capacity reservations of capacity blocks for ML provided by AWS.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # #5045

Special notes for your reviewer:

Checklist:

Release note:

Support running EC2 instances from a reserved capacity block (new API field `marketType`). i.e, marketType: capacity-block

k8s-ci-robot · 2024-11-08T07:41:31Z

Hi @athiruma. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

damdo

Thank you for your PR @athiruma 👍
Left some suggestions to improve the overall readability.

pkg/cloud/services/ec2/instances.go

api/v1beta2/awsmachine_types.go

api/v1beta2/types.go

pkg/cloud/services/ec2/instances_test.go

athiruma · 2024-11-12T03:07:42Z

@damdo??

richardcase · 2024-11-12T08:49:52Z

/ok-to-test

athiruma · 2024-11-12T09:21:29Z

/test pull-cluster-api-provider-aws-test

pkg/cloud/services/ec2/launchtemplate.go

damdo · 2024-11-12T10:45:58Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

damdo · 2024-11-12T11:39:08Z

/test pull-cluster-api-provider-aws-e2e

damdo · 2024-11-12T12:19:33Z

@athiruma The pull-cluster-api-provider-aws-e2e is failing on a the CloudFormation timeout.
We've just increased the timeout in #5193 so if you rebase this PR on the latest main you should pick up the bumped timeout and hopefully this will increase the chances of the e2e to pass.

damdo · 2024-11-12T13:05:09Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

athiruma · 2024-11-12T15:55:53Z

/test pull-cluster-api-provider-aws-e2e-eks

athiruma · 2024-11-12T16:19:51Z

/test pull-cluster-api-provider-aws-e2e-eks

damdo · 2024-11-12T19:43:19Z

The eks test is still flakey. I'll take a look at it tomorrow, I think I know where we can improve it

athiruma · 2024-11-14T11:04:38Z

/test pull-cluster-api-provider-aws-e2e-eks

damdo · 2024-11-14T15:29:34Z

@athiruma see #5215 to improve some issues in the EKS tests

AndiDog · 2024-11-21T17:03:05Z

/lgtm
/retest

Thanks, that was an easy review since I could tick off all done comments so quickly! Let's see if E2E tests pass and then we can merge.

AndiDog · 2024-11-21T17:06:34Z

/test pull-cluster-api-provider-aws-e2e

athiruma · 2024-11-21T17:12:56Z

/test pull-cluster-api-provider-aws-e2e-eks pull-cluster-api-provider-aws-e2e

athiruma · 2024-11-21T17:30:41Z

/test pull-cluster-api-provider-aws-e2e-eks pull-cluster-api-provider-aws-e2e

damdo · 2024-11-21T17:52:35Z

/test pull-cluster-api-provider-aws-e2e-eks pull-cluster-api-provider-aws-e2e

damdo · 2024-11-21T19:14:46Z

/test pull-cluster-api-provider-aws-e2e

athiruma · 2024-11-22T09:22:22Z

/test pull-cluster-api-provider-aws-e2e-eks pull-cluster-api-provider-aws-e2e

damdo · 2024-11-22T15:01:02Z

@athiruma the EKS tests are broken at the moment.
Not sure about the normal E2Es, it looks like it keeps failing on upgrades with that one..
Could this impact that 🤔 or is it a flake?

WDYT @AndiDog

/test pull-cluster-api-provider-aws-e2e

nrb · 2024-11-22T15:23:31Z

It could be that the updates are just longer by the nature of the test, so they're hitting timeouts more often. I don't think the failures are related to this change.

athiruma · 2024-11-24T02:59:29Z

/test pull-cluster-api-provider-aws-e2e-eks pull-cluster-api-provider-aws-e2e

athiruma · 2024-11-25T06:45:10Z

/test pull-cluster-api-provider-aws-e2e

nrb · 2024-11-25T15:19:28Z

/retest

athiruma · 2024-11-28T12:51:06Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

athiruma · 2024-12-01T07:50:44Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

athiruma · 2024-12-09T05:13:22Z

/retest

k8s-ci-robot · 2024-12-09T08:01:23Z

@athiruma: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-aws-e2e	`c975830`	link	false	`/test pull-cluster-api-provider-aws-e2e`
pull-cluster-api-provider-aws-e2e-eks	`c975830`	link	false	`/test pull-cluster-api-provider-aws-e2e-eks`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot · 2024-12-09T15:31:39Z

New changes are detected. LGTM label has been removed.

JoelSpeed · 2024-12-10T12:01:48Z

api/v1beta2/awsmachine_types.go

+	// UseCapacityBlock enables usage of pre-purchased compute capacity (capacity blocks) with AWS Capacity Reservations.
+	// If enabled, CapacityReservationID must be specified to identify the target reservation.
+	// +optional
+	UseCapacityBlock *bool `json:"useCapacityBlock,omitempty"`


Why is this needed, if all it is doing is enabling the capacity reservation ID field?

we need this to use for AWS capacity blocks for ml.

JoelSpeed · 2024-12-10T12:03:41Z

pkg/cloud/services/ec2/instances.go

+		return &ec2.InstanceMarketOptionsRequest{
+			MarketType: aws.String(ec2.MarketTypeCapacityBlock),
+		}, nil


Why not make the exposed API mirror this in a way that we can expand the values in the future, without having to have lots of UseXYZ kinds of fields?

Would be much better to expose an enum, bools don't age well

we have 3 marketType options here: spot, on-demand and capacity-block where we can omit on-demand.

@JoelSpeed How about now?
Adding the MarketType API field. It can be set to two values spot/ capacity-block. If not specified and SpotMarketOptions is specified default we will set MarketType to capacity-block

athiruma · 2024-12-11T04:39:56Z

@damdo ptal ?

JoelSpeed · 2024-12-18T11:12:59Z

api/v1beta2/awsmachine_types.go

@@ -197,6 +197,14 @@ type AWSMachineSpec struct {
 	// CapacityReservationID specifies the target Capacity Reservation into which the instance should be launched.
 	// +optional
 	CapacityReservationID *string `json:"capacityReservationId,omitempty"`
+
+	// MarketType specifies the type of market for the EC2 instance. Valid values include:


Suggested change

// MarketType specifies the type of market for the EC2 instance. Valid values include:

// marketType specifies the type of market for the EC2 instance. Valid values include:

JoelSpeed · 2024-12-18T11:13:21Z

api/v1beta2/awsmachine_types.go

+	//  If this value is selected, CapacityReservationID must be specified to identify the target reservation.
+	// If MarketType is not specified and SpotMarketOptions is provided, the MarketType defaults to "spot".
+	// +optional
+	MarketType *MarketType `json:"marketType,omitempty"`


Does this need to be a pointer?

It should allow empty values so I thought we should use a pointer here.

Do you care about the difference between marketType: "" and the field not being present at all? The controller will treat those two representations in the same way won't it?

Hmm, I see then I'll make a change. Thanks

JoelSpeed · 2024-12-18T11:14:13Z

api/v1beta2/types.go

+const (
+	// MarketTypeOnDemand is a MarketType enum value
+	MarketTypeOnDemand MarketType = "on-demand"
+
+	// MarketTypeSpot is a MarketType enum value
+	MarketTypeSpot MarketType = "spot"
+
+	// MarketTypeCapacityBlock is a MarketType enum value
+	MarketTypeCapacityBlock MarketType = "capacity-block"
+)


Kubernetes API conventions state that enum values should be PascalCase. We prefer this and then to convert to upstream APIs (EC2) in this case so that there's consistency across Kube like APIs. So these should really be OnDemand, Spot and CapacityBlock

JoelSpeed · 2024-12-18T11:14:46Z

api/v1beta2/types.go

 }

+// MarketType describes the market type of an Instance
+type MarketType string


You'll need a kubebuilder:validation:Enum marker on here to make sure the correct schema is generated

enxebre · 2024-12-18T11:55:27Z

api/v1beta2/awsmachine_webhook.go

@@ -361,6 +362,14 @@ func (r *AWSMachine) validateNetworkElasticIPPool() field.ErrorList {
 	return allErrs
 }

+func (r *AWSMachine) validateInstanceMarketType() field.ErrorList {
+	var allErrs field.ErrorList
+	if ptr.Deref(r.Spec.MarketType, "") == MarketTypeCapacityBlock && r.Spec.SpotMarketOptions != nil {


shouldn't this also check MarketType is not ondemand if SpotMarketOptions != nil? i.e. the check should be that the only valid when SpotMarketOptions != nil is MarketType==spot

oh yeah good catch thanks

enxebre · 2024-12-18T12:00:11Z

api/v1beta2/awsmachine_types.go

@@ -197,6 +197,14 @@ type AWSMachineSpec struct {
 	// CapacityReservationID specifies the target Capacity Reservation into which the instance should be launched.
 	// +optional
 	CapacityReservationID *string `json:"capacityReservationId,omitempty"`
+
+	// MarketType specifies the type of market for the EC2 instance. Valid values include:


Can we describe in gdoc what's the default behaviour if I don't set spot or capacityBlock? i.e on demand

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 8, 2024

k8s-ci-robot requested review from dlipovetsky and richardcase November 8, 2024 07:41

damdo reviewed Nov 8, 2024

View reviewed changes

athiruma force-pushed the add_instance_market_option branch from 8ae7c7b to d34e59b Compare November 8, 2024 19:16

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 8, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 12, 2024

damdo reviewed Nov 12, 2024

View reviewed changes

pkg/cloud/services/ec2/launchtemplate.go Outdated Show resolved Hide resolved

athiruma force-pushed the add_instance_market_option branch from d34e59b to f21fca7 Compare November 12, 2024 09:51

athiruma force-pushed the add_instance_market_option branch from f21fca7 to 05baf2d Compare November 12, 2024 12:52

AndiDog changed the title ~~✨ Added the support for capacity blocks~~ ✨ Add the support for capacity blocks Nov 21, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2024

Added the support for capacity blocks

6c1c6bd

athiruma force-pushed the add_instance_market_option branch from c975830 to 6c1c6bd Compare December 9, 2024 15:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2024

JoelSpeed reviewed Dec 10, 2024

View reviewed changes

athiruma force-pushed the add_instance_market_option branch from b5997f8 to 7a2948f Compare December 12, 2024 15:33

JoelSpeed reviewed Dec 18, 2024

View reviewed changes

enxebre reviewed Dec 18, 2024

View reviewed changes

new API field marketType

c1d94dc

athiruma force-pushed the add_instance_market_option branch from 7a2948f to c1d94dc Compare December 20, 2024 15:26

	// MarketType specifies the type of market for the EC2 instance. Valid values include:
	// marketType specifies the type of market for the EC2 instance. Valid values include:

✨ Add the support for capacity blocks #5211

Are you sure you want to change the base?

✨ Add the support for capacity blocks #5211

Conversation

athiruma commented Nov 8, 2024 • edited Loading

k8s-ci-robot commented Nov 8, 2024

damdo left a comment

Choose a reason for hiding this comment

athiruma commented Nov 12, 2024

richardcase commented Nov 12, 2024

athiruma commented Nov 12, 2024

damdo commented Nov 12, 2024

damdo commented Nov 12, 2024

damdo commented Nov 12, 2024

damdo commented Nov 12, 2024

athiruma commented Nov 12, 2024

athiruma commented Nov 12, 2024

damdo commented Nov 12, 2024

athiruma commented Nov 14, 2024

damdo commented Nov 14, 2024

AndiDog commented Nov 21, 2024

AndiDog commented Nov 21, 2024

athiruma commented Nov 21, 2024

athiruma commented Nov 21, 2024

damdo commented Nov 21, 2024

damdo commented Nov 21, 2024

athiruma commented Nov 22, 2024

damdo commented Nov 22, 2024

nrb commented Nov 22, 2024

athiruma commented Nov 24, 2024

athiruma commented Nov 25, 2024

nrb commented Nov 25, 2024

athiruma commented Nov 28, 2024

athiruma commented Dec 1, 2024

athiruma commented Dec 9, 2024

k8s-ci-robot commented Dec 9, 2024

k8s-ci-robot commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athiruma commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athiruma commented Nov 8, 2024 •

edited

Loading

athiruma commented Dec 11, 2024 •

edited

Loading

enxebre Dec 18, 2024 •

edited

Loading